feat: add notification system (for blocks) - #1475
Conversation
7e798a5 to
89c2813
Compare
|
displays a bell that will link to a new notification page |
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
A few nits / comments inline.
| if person is None: | ||
| return Response({"count": 0}) | ||
| seen_at = self._seen_at(person) | ||
| unread = Q() if seen_at is None else Q(created__gt=seen_at) |
There was a problem hiding this comment.
Just a style comment (not even a nit really): IMO this would be easier to read if it were written out as something like unread = self.get_queryset() if seen_at is None else self.get_queryset().filter(created_at...) instead of by passing Q() objects around. (Really, I'd probably write it out as a multi-line if statement)
I'm not really asking for a change, just noticed this and have been bitten by operations on Q objects being counterintuitive at times. For readability / maintainability, sometimes it's better to be a little wordier or use additional lines of code.
| ) | ||
| # Denormalized event detail (draft name, blocking reason names) so a | ||
| # notification renders without re-deriving state that may since have changed. | ||
| data = models.JSONField(default=dict, blank=True) |
There was a problem hiding this comment.
Might consider separately modeling draft_name (as a char field) and reasons (as an arrayfield or a JSONField that's a list) to reduce the amount of denormalized schema. The serializer code is depending on the structure of this denormalized data which is a hint that a JSONField is creating a hidden maintenance burden.
| @@ -0,0 +1,53 @@ | |||
| import { ref, onBeforeMount, onUnmounted, readonly } from 'vue' | |||
There was a problem hiding this comment.
missing copyright stmt
|
|
||
| const markAllRead = async () => { | ||
| if (!api) return | ||
| await api.notificationsMarkRead() |
There was a problem hiding this comment.
unhandled exception opportunity - is there a sensible thing to do if this API call fails?
| def get_queryset(self): | ||
| user = self.request.user | ||
| if not user.is_authenticated: | ||
| return Notification.objects.none() |
There was a problem hiding this comment.
I don't think this API is exposed for non-authenticated users. Might consider raising an exception or emitting a log if a non-authenticated user gets here as an early warning that something is not working as expected. (OTOH that might be better covered by a test that does a more systematic confirmation that our auth system is operating as intended and doing it at an API-by-API level is not desirable)
2 participants
fix #1426
implement a simple notification/broadcast system:
newnotificationsnotificationpage lists all past notificationsSystem is versatile and can be user for more use-cases later